[cDAC] SOSDacImpl::GetMethodDescData DynamicMethodObject #110545
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
max-charlamb
requested review from
mikem8361,
tommcdon,
AaronRobinsonMSFT,
elinor-fung and
a team
|
Tagging subscribers to this area: @tommcdon |
11 tasks
mikem8361
reviewed
Dec 9, 2024
| @@ -405,7 +405,8 @@ int ISOSDacInterface.GetMethodDescData(ulong methodDesc, ulong ip, DacpMethodDes | |||
| Debug.Assert(data->MDToken == dataLocal.MDToken); | |||
| Debug.Assert(data->GCInfo == dataLocal.GCInfo); | |||
| Debug.Assert(data->GCStressCodeCopy == dataLocal.GCStressCodeCopy); | |||
| Debug.Assert(data->managedDynamicMethodObject == dataLocal.managedDynamicMethodObject); | |||
| // managedDynamicMethodObject is not currently populated by the cDAC API and may differ from legacyImpl. | |||
There was a problem hiding this comment.
Where is the managedDynamicMethodObject field actually zero'ed? I don't see it anywhere. I'm not sure if you should rely on the client to zero it (like in dacprivate.h). It may not be enough i.e. CLRMD doesn't use that include file.
There was a problem hiding this comment.
I see. I wasn't sure if the return memory region would be zero'd to begin with, but that makes sense. I'll zero out the field to make sure it is always returned with 0.
added 2 commits
December 9, 2024 16:52
mikem8361
approved these changes
Dec 9, 2024
elinor-fung
approved these changes
Dec 9, 2024
This was referenced Dec 10, 2024
Open
|
/ba-g Build analysis blocked by #110285 |
hez2010
pushed a commit
to hez2010/runtime
that referenced
this pull request
Dec 14, 2024
* allow SOSDacImpl::GetMethodDescData to handle dynamic functions * zero-out managedDynamicMethodObject as it is not being used and cDAC does not yet support fetching managed fields
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Contributes to #109426
Previously the cDAC version of
GetMethodDescDatawould throw if a method is dynamic because fetching themanagedDynamicMethodObjectwas not supported. ThemanagedDynamicMethodObjectis a small portion of the returned information for dynamic methods and is unused in both SOS and CLRMD.Since it is a relatively large lift to access this field (on a CorLib bound managed object) I believe it is reasonable to leave this field blank for now. Due to compatibility with the legacy implementation we need to keep the return object the same so the field is zero'd out.